fix(py): Fix array/list value serialization #1827
Conversation
|
Will |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1827 +/- ##
==========================================
- Coverage 86.64% 86.63% -0.01%
==========================================
Files 192 192
Lines 34754 34758 +4
Branches 31571 31571
==========================================
Hits 30113 30113
- Misses 2940 2944 +4
Partials 1701 1701
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -60,7 +60,7 @@ class ArrayVal(val.ExtensionValue): | |||
| """Constant value for a statically sized array of elements.""" | |||
|
|
|||
| v: list[val.Value] | |||
| ty: tys.Type | |||
| ty: Array | |||
There was a problem hiding this comment.
this is non-breaking because Array satisfies the Type protocol, correct?
There was a problem hiding this comment.
It's fine for reading since it's a type refinement.
For writing into the variable it is actually a breaking change. I assumed the dataclass was frozen, but it seems it's not.
Anyways I'd say mutating the attribute directly is not common (since the type is normally constructed using its custom __init__), and putting in a non-array type should be a bug anyways.
|
have checked this test passes with this change: https://github.com/quantinuum-dev/guppy-integration/blob/91cb4b95302c69cc8c2933a00aee8dc21974e271/tests/test_simple.py#L147 |
🤖 I have created a release *beep* *boop* --- ## [0.10.2](hugr-py-v0.10.1...hugr-py-v0.10.2) (2024-12-20) ### Features * Add `StringVal` to hugr-py ([#1818](#1818)) ([b05a419](b05a419)) ### Bug Fixes * **py:** Fix array/list value serialization ([#1827](#1827)) ([7bf85b9](7bf85b9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
The rust-side serialization definition for custom constants is a bit more complex than what we had written on the python side for array and list values.
This fixes #1826, and was tested by running the issue's example from guppy.
Constant value deserialization is a "best effort" thing.
If we can match the serialized thing with one of the hardcoded implementors of
CustomConst(registered in a global var bytypetag) then we deserialize into the specific class.If we cannot deserialize into any specific class, we use
CustomSerializedby default.So even if we build a constant from python and
validateusing the rust checker, the deserialization will succeed and validate correctly.It'd be nice to fail the deserialization if a constant claims to be of some specific kind but cannot be deserialized into it. I'll open an issue and look a bit into it.